-
Notifications
You must be signed in to change notification settings - Fork 356
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: refactor experiment creation modal and add continue workflow #971
Conversation
export interface CreateExpModalState { | ||
visible: boolean; | ||
config: string; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: these two states don't really have anything in common, and by that I mean that when you call setState
, you are only really changing one or the other and not the two simultaneously. Rather than bundling them into a separate interface, it'll be easier to handle code that treats them separately.
interface Props {
...
config: string;
onCancel: () => void;
onConfigChange: () => string;
visible?: boolean;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is another concern of passing in a function that can mutate a state of a different component directly that's not itself instead of a buffered event handler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm I had it the other way first but here I'm thinking we could separate writeable states of the component into its own group as I'm anticipating we add more when we add the form on top or more features.
update: I separated the read part of the states into separate props but as we talked I left the set part of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left out the part that creates another handler on top of the setState to let the interface of the setState which allows both passing the new state and a function to set the state but also lower changes to experiment details page as you already have another PR open there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This worked great when I ran it!
Some suggestions and nits on rearranging the component to use more basic component patterns.
e99e43d
to
7c51222
Compare
1d3181d
to
4a7b47b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's maybe zoom about this PR.
parentId: number; | ||
visible: boolean; | ||
config: string; | ||
setState: (arg0: SetStateAction<CreateExpModalState>) => void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: TL;DR - avoid passing setState
to child component and instead pass in a setState
The crux problem of this anti-pattern is that the state writing operation is in two places, the parent and the child component. The on[Action]
pattern is used to keep the state and setState contained in the parent and the child modal will just send events back to the parent for it to handle the source of truth (state). It's also perfectly fine to have multiple on[Action]
handler because the idea there is to orthogonalize unrelated events from each other, so the handlers do not have to worry about one event from another. This is especially important with the useCallback
hook, because of the dependency list.
const [ ContModalState, setContModalState ] = useState<CreateExpModalState>( | ||
{ config: 'Loading', visible: false }, | ||
); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: separate these into two different useState
. They are related in that they are both fed into the modal but that's about it. Changes in visibility and changes in config shouldn't alter each other.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that, I can separate them. let's chat about it. Wouldn't react be smart enough to not cause rerenders if only one of these changes? On the other hand, I remember adding each individual hook adds an extra rerender.
3f5081b
to
132ed48
Compare
onVisibleChange: (arg0: boolean) => void; | ||
onConfigChange: (arg0: string) => void; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super nit: visible
and config
as alternative to arg0
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really good! One tiny non-blocking nit.
to be more consistent with common patterns?
78afa4d
to
e3476cc
Compare
Description
as part of https://determinedai.atlassian.net/browse/DET-3703
this excludes the helper form that sets max steps and description
Test Plan
Commentary (optional)
an important part of this is to create the new experiment config based on the trial and that happens here
AFAIK these are the only fields we need to update. @katport does this sound, right?